Skip to content

gh-129013: Don't expose async_generator_wrapped_value in PY_YIELD callbacks #129031

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

gh-129013: Don't expose async_generator_wrapped_value in PY_YIELD callbacks #129031

wants to merge 6 commits into from

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Jan 19, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Jan 19, 2025
Open
@vstinner
Copy link
Member

I don't know these parts of the Python code base, I cannot review your change.

@agronholm
Copy link
Contributor

Thanks for this PR, I just found my way here as I was hit by the same bug!

agronholm
agronholm reviewed Apr 3, 2025
View reviewed changes
Copy link
Contributor

@agronholm agronholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a couple of comments and a question: how is a user supposed to distinguish between yield and await in the callback?

Lib/test/test_monitoring.py
Comment on lines +2107 to +2108
asyncio.run(main())
sys.monitoring.set_events(0, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would seem slightly cleaner.

Suggested change
asyncio.run(main())
sys.monitoring.set_events(0, 0)
try:
asyncio.run(main())
finally:
sys.monitoring.clear_tool_id(0)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not disagreeing, but I'd rather stay consistent with the rest of the file. try/finally doesn't seem to be used with set_events(0, 0) in these tests, so I'm going to skip it for now. Sounds reasonable to add that in a follow-up PR, though.

@ZeroIntensity
Copy link
Member Author

how is a user supposed to distinguish between yield and await in the callback?

Hmm, I guess you could check if its an instance of asyncio.Future, assuming the user isn't yielding those for any other reason. Normal coroutines have this issue too; I guess it might be worth adding a PY_AWAIT event?

@agronholm
Copy link
Contributor

agronholm commented Apr 3, 2025 •
edited
Loading

how is a user supposed to distinguish between yield and await in the callback?

Hmm, I guess you could check if its an instance of asyncio.Future, assuming the user isn't yielding those for any other reason. Normal coroutines have this issue too; I guess it might be worth adding a PY_AWAIT event?

Futures being awaited are not the issue – it's things like asyncio.sleep(0) which just yield None. This would be indistinguishable from a plain yield. I'm just not sure how to fix this w/o breaking backwards compatibility.

@agronholm
Copy link
Contributor

I do agree that ideally we should have separate PY_YIELD and PY_AWAIT.

@agronholm
Copy link
Contributor

Perhaps the easiest backwards compatible solution is to expose the wrapper type and its contained value to Python code? @vstinner any thoughts so far?

@ZeroIntensity
Copy link
Member Author

I would really rather not expose our implementation details to Python code :(

Do other implementations support sys.monitoring? Perhaps we could take inspiration from them.

@agronholm
Copy link
Contributor

I would really rather not expose our implementation details to Python code :(

Do other implementations support sys.monitoring? Perhaps we could take inspiration from them.

sys.monitoring was added in 3.12 and both PyPy and GraalPy are still at 3.11, so no.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@vstinner vstinner Awaiting requested review from vstinner

1 more reviewer

@agronholm agronholm agronholm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ZeroIntensity @vstinner @agronholm